fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950) - #71743

Both points addressed in c7869e1f4 (rebased onto current main first).

Discovery now rides the supervisor connection. Correct — Target.getTargets had no target_id, skipped the routing branch, and still opened a fresh per-call WebSocket, so on Browserless the discovery result described a different private browser than the one a follow-up target_id call executed in. _browser_cdp_target_via_supervisor now accepts target_id=None and dispatches browser-level commands (no sessionId) on the supervisor's WebSocket — _cdp already treats session_id as optional, so this is the same connection either way — and the call site routes every post-guard call through it when a live supervisor exists. The requested per-WebSocket-isolation test is a real-Chrome integration test covering the full chain: the stateless _cdp_call is patched to fail the test if anything reaches it, then Target.getTargets (asserted to include the supervisor's own attached page — proof both calls observe one browser) followed by Runtime.evaluate on the discovered target_id, both succeeding over the supervisor connection. Unit tests cover the browser-level dispatch shape (no sessionId on the wire) and the no-supervisor stateless fallback.

Mixed daemon/supervisor behavior — defined and documented. The semantics this PR now commits to, of your three options, are verified-shared-connection with stateless fallback:

• Live supervisor → all browser_cdp traffic (discovery + target-scoped) shares its connection, so results are mutually coherent by construction; responses carry "connection": "supervisor" so callers can see which regime they're in.
• No supervisor, or a target_id the supervisor has no session for → legacy stateless connection, unchanged (the plain-Chrome path, where every connection sees the shared browser).

On the second half — page_target_id describing the supervisor's independently-connected browser rather than the agent-browser daemon's page: that's exact, and it's the supervisor re-attach root cause we split into #74216 (wrong-page dialog bridge + snapshot enrichment; consolidates the #32685 repro, the plain-Chrome multi-tab confirmation on #71744, and the re-attach direction — a contributor is sequencing that work after #71745). Fixing it belongs at the supervisor attach layer, not per-call in browser_cdp; until it lands, browser.md now states explicitly that the supervisor's browser is not necessarily the daemon's page and that within browser_cdp supervisor state is the canonical view, with a pointer to the tracking issue. Happy to re-scope if you'd rather this PR waits on #74216, but the eval-side companion (#71745) took the same boundary per the triage recommendation.

This was generated by AI during triage.

Summary

Nine PRs span two distinct causes: #32950 and #71743 address #32685 by reusing a persistent CDP supervisor connection, while #4143, #6623, #27304, #30893, #33246, #34131, and #34497 address execute_code terminal exposure, environment leakage, approval gating, or lost thread context. The execute_code cluster is already consolidated in merged #34497; the remaining open issue is the Browserless per-WebSocket lifecycle handled by the two browser PRs.

Related pull requests

• fix(security): remove terminal from sandbox allowed tools in execute_code #4143 [closed] partial — (+338/-222) — n/a: Removes terminal from execute_code, closing one call path but not the confirmed lost approval-context cause; closed after a contributor rejected terminal revocation as the architectural fix, and retained only as security-cluster history.
• fix(tools): revoke terminal access within execute_code sandbox to prevent escape #6623 [closed] partial — (+58/-49) — n/a: Duplicates fix(security): remove terminal from sandbox allowed tools in execute_code #4143's terminal-revocation approach without addressing RPC thread context or browser_cdp persistence; closed and relevant only as the same superseded proposal.
• fix: harden execute_code env and approvals #27304 [closed] fixes — (+102/-10) — n/a: Adds explicit child-environment filtering and an RPC-boundary terminal guard, but not complete ContextVar and callback propagation; superseded by merged fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497, which incorporated its env-scrub direction.
• [codex] Guard execute_code behind gateway approvals #30893 [closed] fixes — (+406/-66) — n/a: Adds the fail-closed whole-script execute_code guard for gateway, ask, and cron-deny contexts, covering direct Python escape paths; superseded by merged fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497, which combined that guard with RPC context restoration.
• fix(tools): reuse supervisor sessions for browser_cdp target_id #32950 best fix — (+121/-0) — best fix: Introduces the core target-ID-to-live-supervisor-session reuse for [Bug]: browser_cdp opens stateless CDP connections, breaking Browserless/BaaS targets between tool calls #32685, but its keep_open review identifies guard-ordering, private discovery, and redaction gaps still present in this diff; fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950) #71743 preserves the mechanism and adds those fixes plus same-connection discovery coverage.
• [codex] Preserve context in execute_code RPC threads #33246 [closed] partial — (+62/-4) — n/a: Propagates ContextVars into both execute_code RPC threads but omits thread-local approval and sudo callbacks; superseded by fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497's complete shared propagation helper.
• fix(security): restore approval/sudo context in execute_code RPC threads + guard entry points #34131 [closed] fixes — (+827/-151) — n/a: Implements the confirmed execute_code root-cause fix—full context/callback propagation, entry gating, and environment hardening—but was closed as redundant after the same implementation landed through fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497.
• fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497 [merged] best fix — (+897/-151) — best fix: Merged reference implementation for Critical: Sandbox code execution bypasses dangerous command approval via terminal tool #4146, execute_code can leak common secret env vars and bypass terminal approval guard #27303, [Bug]: [Security] approvals.mode=manual silently bypassed in gateway mode — dangerous commands execute without approval, including via execute_code sandbox evasion #30882, and fix(cli): dangerous command approval bypass in execute_code RPC threads #33057, combining complete RPC context/callback propagation, fail-closed script gating, and hardened environment scrubbing; it remains relevant as the landed consolidation, not as a browser_cdp fix.
• fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950) #71743 best fix — (+522/-10) — best fix: Salvages fix(tools): reuse supervisor sessions for browser_cdp target_id #32950 and routes both Target.getTargets discovery and target-scoped calls over one supervisor WebSocket, with private-page guarding, public target discovery, redaction, stateless fallback, and a discovery-to-evaluate regression test. The visible keep_open review should remain controlling until contributor re-review confirms these changes and accepts the documented split of mixed daemon/supervisor lifecycle work to [Bug]: CDPSupervisor attaches to an arbitrary page and is never re-pointed — dialog bridge and snapshot enrichment target the wrong page (daemon/supervisor split-brain) #74216.

Duplicates

#4143 and #6623 duplicate the terminal-revocation approach. #27304, #30893, #33246, and #34131 are superseded precursors consolidated in merged #34497; #32950 and #71743 share the browser supervisor-reuse mechanism, with #71743 as the expanded salvage.

Suggested consolidation

Keep #71743 open with a salvage path: preserve its same-supervisor discovery-to-evaluate chain, guard, redaction, public snapshot field, fallback tests, and attribution, then obtain contributor re-review of the still-visible keep_open verdict. Despite the keep_open review and recorded best-fix status on #32950, close #32950 as duplicate of #71743 because its diff retains the three cited blockers while #71743 carries its core fix and addresses them; no further action is needed for the execute_code PRs because merged best-fix #34497 is their consolidated reference implementation.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I32685(["issue #32685 (open)"])
    subgraph Dup32950 ["PRs duplicating each other"]
        P32950["PR #32950 (open)"]
        P71743["PR #71743 (open)"]
    end
    P71743 -->|best fix| I32685
    class I32685 open
    class P32950 open
    class P71743 open
    class P32950 best
    class P71743 best
    class P71743 target
    click I32685 "https://github.com/NousResearch/hermes-agent/issues/32685"
    click P32950 "https://github.com/NousResearch/hermes-agent/pull/32950"
    click P71743 "https://github.com/NousResearch/hermes-agent/pull/71743"

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 9 pull requests and 5 issues in this complex. Each diff was read against this issue; Assessment working set: 239 kB of PR diffs, 39 kB of issue/PR text, 23 kB of discussion (25 comments), 38 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Thanks — 1 and 4 documented in 6411c34f5; 2 and 3 answered below. No behavior change; unit suite 17 green (test_browser_supervisor.py remains marker-gated as before).

1 (timeout + 2 vs safe_timeout). The two paths' latency contracts are already symmetric at the value level — both receive the same clamped safe_timeout from the tool entrypoint. The +2 is only the outer future guard: the inner supervisor._cdp(...) enforces timeout itself, so the margin exists to let the inner, more specific CDP timeout error surface instead of a generic fut.result timeout, and it only fires if the loop itself is wedged. Now stated in a comment at both call sites.

2 (unlocked _loop/_cdp reads) — declining the lock. As you note, a torn read is caught and falls back to the stateless path, so the worst case is one call taking the slow path. Acquiring _state_lock on the tool hot path would buy determinism nobody observes at the cost of contention with the supervisor's event thread.

3 (plain-Chrome browser-level behavior change). Confirmed no caller depends on stateless isolation: browser_cdp has no internal callers (only its own registry handler), and on plain Chrome targets are shared across connections, so browser-level discovery returns the same data either way. The only observable difference is on per-connection-browser backends — where riding the supervisor's connection is precisely the fix (#32685). The docs note covers it.

4 (partial attach) — comment added at the resolve_target_session early return: _page_target_id known but _page_session_id not yet ⇒ deliberate None, so callers take the stateless fallback rather than racing a half-attached session.

Status: triage marks this best fix — fixes for the #32685 family; browser-level calls ride the supervisor WebSocket since c7869e1f4, review points documented in 6411c34f5. Rebased onto upstream main daily; re-verified today against the v0.20.5 release tag (v2026.8.19) — cherry-picks clean, test_browser_cdp_tool.py green (test_browser_supervisor.py remains marker-gated for CI). The graft-onto-#32950 offer from the consolidation thread stands — happy to go either route. Ready for review or salvage-merge whenever convenient.

Re-homed onto current main after #102117 (the whole-codebase simplification) landed this morning and left this branch conflicting — head 87403628f2, mergeable again. Port notes: both files survived the split; resolve_target_session now checks the page target then _frames (upstream dropped _child_sessions), and the supervisor payload uses the new _redact_cdp_output(always_paths=…, flagged_paths=…) signature. Unit 30 passed; real-Chromium integration 9 passed / 1 skipped. git diff main...HEAD audited to contain only this PR's files; no behavior change vs. the pre-refactor version.

PR body rewritten in the Symptom → change → behavior shape. If cherry-picking/salvaging is the faster path for this one, please go ahead — no objection to authorship going either way.